Skip to content

Changes for session and schema #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 24, 2022
Merged

Conversation

mjok
Copy link
Contributor

@mjok mjok commented Mar 10, 2022

This need a discussion first.
Background liquibase cassandra uses outdated Simba JDBC driver; This project would be perfect fit for replacing it.

First thing, there is different concepts about catalog, You use cluster as catalog, as liquibase (simba) expects keyspace. And thus ailing on some checks:
getColumns <..>
if (catalog != null && !catalog.equals(this.connection.getCatalog()))

The solution could be either make this configurable, or suggest another solution.

Update also need review. Your return 0, with -1 "liquibase cassandra" would work out of the box. Yet again, this can be made configurable.

And passing a session would benefit. Actually I'd like to use the session from my application, there all the security params already configured, and there is no need to open additional session, as I have already one Idle.

note:

    private final TreeSet<String> hostListPrimary;
    private final TreeSet<String> hostListBackup;

seems to be unused variables

**sorry made a mess with previous pull request

@mjok mjok marked this pull request as draft March 10, 2022 11:26
@maximevw
Copy link
Collaborator

Thank you for taking the time to suggest improvements to this library.
I will answer your first comment below.

This need a discussion first. Background liquibase cassandra uses outdated Simba JDBC driver; This project would be perfect fit for replacing it.

First thing, there is different concepts about catalog, You use cluster as catalog, as liquibase (simba) expects keyspace. And thus ailing on some checks: getColumns <..> if (catalog != null && !catalog.equals(this.connection.getCatalog()))

The solution could be either make this configurable, or suggest another solution.

I perfectly understand your point. We kept the following correspondance of concepts as it existed in the library originally developed by @adejanovski (https://github.com/adejanovski/cassandra-jdbc-wrapper).
We think this is the most accurate correspondance regarding the definition of JDBC concepts.

JDBC concept Correspondance in Cassandra Correspondance in a SQL database
Catalog Cluster Database or package
Schema Keyspace Schema
Table Table Table

So, if we consider that the catalog concept matches the Cassandra keyspace, what about the schema concept? Should we match it with the keyspace too? Why not... if we make it configurable to fit different needs like being compatible with Liquibase Cassandra. But I'm not sure it makes it clear to use it as the default implementation. I'll come back to it later.

Update also need review. Your return 0, with -1 "liquibase cassandra" would work out of the box. Yet again, this can be made configurable.

Even if we'll never be JDBC compliant because Cassandra is obviously not SQL 92 compliant 😉, we try to stick as more as possible to the JDBC 4.3 API specifications.

So, regarding the method executeUpdate(), the specification mention (page 95):

13.1.2.2 Returning an Update Count

In CODE EXAMPLE 13-5, the SQL statement being executed returns the number of rows affected by the update for SQL Data Manipulation Language (DML) statements or 0 for SQL statements that return nothing.

So, we expect to return 0 since the driver doesn't return the number of updated rows.

But, once again, for specific needs, we could make it configurable. One solution would be to introduce a parameter complianceMode=liquibase | default (names can be discussed) in the JDBC URL indicating we want to deviate from the standard implementation to use a specific one. For example, if we want to be compliant with Liquibase Cassandra, we set the parameter value to liquibase. Then, instead of instantiating the default implementation classes (like CassandraStatement), it will used overridden classes (e.g. LiquibaseCassandraStatement) with specific behaviors. It's an idea, we can discuss it to avoid unecessary complexity.

And passing a session would benefit. Actually I'd like to use the session from my application, there all the security params already configured, and there is no need to open additional session, as I have already one Idle.

I don't see any inconvenient to add such a constructor.

note:

    private final TreeSet<String> hostListPrimary;
    private final TreeSet<String> hostListBackup;

seems to be unused variables

You're right, probably an oversight after a big refactoring :) We can remove them.

@maximevw
Copy link
Collaborator

Please note I cleaned the unused code mentioned in your comment on the branch release/next.

@mjok
Copy link
Contributor Author

mjok commented Mar 23, 2022

Have no idea why build pipeline fails and
I wasn't able to run tests...

java.lang.IllegalAccessError: superclass access check failed: class org.apache.cassandra.utils.JMXServerUtils$JmxRegistry (in unnamed module @0x3498ed) cannot access class sun.rmi.registry.RegistryImpl (in module java.rmi) because module java.rmi does not export sun.rmi.registry to unnamed module @0x3498ed

@maximevw
Copy link
Collaborator

Here is what I got in the build pipeline:

[INFO] Running com.ing.data.cassandra.jdbc.MetadataResultSetsUnitTest
[ERROR] Tests run: 6, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 3.227 s <<< FAILURE! - in com.ing.data.cassandra.jdbc.MetadataResultSetsUnitTest
[ERROR] givenStatement_whenMakeSchemas_returnExpectedResultSet  Time elapsed: 0.012 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <embedded_test_cluster> but was: <test_keyspace>
	at com.ing.data.cassandra.jdbc.MetadataResultSetsUnitTest.givenStatement_whenMakeSchemas_returnExpectedResultSet(MetadataResultSetsUnitTest.java:89)

[ERROR] givenStatement_whenMakeCatalogs_returnExpectedResultSet  Time elapsed: 0.001 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <embedded_test_cluster> but was: <test_keyspace>
	at com.ing.data.cassandra.jdbc.MetadataResultSetsUnitTest.givenStatement_whenMakeCatalogs_returnExpectedResultSet(MetadataResultSetsUnitTest.java:72)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   MetadataResultSetsUnitTest.givenStatement_whenMakeCatalogs_returnExpectedResultSet:72 expected: <embedded_test_cluster> but was: <test_keyspace>
[ERROR]   MetadataResultSetsUnitTest.givenStatement_whenMakeSchemas_returnExpectedResultSet:89 expected: <embedded_test_cluster> but was: <test_keyspace>
[INFO] 
[ERROR] Tests run: 182, Failures: 2, Errors: 0, Skipped: 0

I changed the configuration of the CI pipeline in Github actions to also build pull requests branches (I just updated the branch release/next).

I guess the error you mentioned comes from your local build. This error could happen if you run tests with JDK 9+ and not allowing illegal accesses. Try to build using JDK8 or tell me how you launched the tests.

@mjok
Copy link
Contributor Author

mjok commented Mar 23, 2022

Yes, you're right. JDK 1.8 works fine.
And had to shutdown Cassandra I was running locally. My bad.

Glad it worked, find some quirkies I've lost at first.

I've left with one failure,
in givenEnabledSslWithJsse_whenConfigureSsl_addDefaultSslEngineFactoryToSessionBuilder
Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 2: /e:/workspace/cassandra-jdbc-wrapper/target/test-classes/test_truststore.jks
I can see why, didn't bother to change that.

I can see still Build pipeline failure

@maximevw
Copy link
Collaborator

Do you see the pipeline logs here: https://github.com/ing-bank/cassandra-jdbc-wrapper/actions/workflows/ci-workflow.yml ?

For Azure pipeline, I restarted it, it failed when it checked-out the repo from Github... and now it succeeded.

@mjok
Copy link
Contributor Author

mjok commented Mar 24, 2022

Yes, I can.

@mjok mjok marked this pull request as ready for review March 24, 2022 07:18
@mjok mjok changed the title DRAFT: Changes for session and schema Changes for session and schema Mar 24, 2022
Copy link
Collaborator

@maximevw maximevw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be compliant with our contributing policy, please:

  • add license headers in the new Java files
  • add Javadoc on the public methods and constructors (you can find Checkstyle findings in the build logs)
  • add unit tests for the new code each time it is possible

Thank you

Copy link
Collaborator

@maximevw maximevw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mjok for your contribution!
Do not hesitate to create new discussions / pull requests if you detect new potential improvements.

@maximevw maximevw merged commit 4fcbf62 into ing-bank:release/next Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants